Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arm64: change some format #14991

Merged
merged 1 commit into from
Nov 29, 2024
Merged

arm64: change some format #14991

merged 1 commit into from
Nov 29, 2024

Conversation

hujun260
Copy link
Contributor

Summary

arm64: change some format

fix the comment in #14980

Impact

arm64

Testing

ci ostest

@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small labels Nov 29, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 29, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details.

Here's a breakdown of what's missing:

  • Summary:

    • Lacks a clear explanation of why the format change is necessary. Is it for readability? Consistency? Correctness? Just saying "fix the comment" isn't sufficient. What was wrong with the comment?
    • Needs to specify which functional part of the code is being changed. "arm64" is too broad. Is it in a specific driver? A core architecture file?
    • Needs to explain how the change works. "change some format" is vague. Provide specifics about the formatting changes.
  • Impact:

    • Simply stating "arm64" doesn't explain the impact. The Impact section requires YES/NO answers for each category. Even if the answer is NO, it should still explicitly state NO. If the answer is YES, a description is required. For example:
      • Impact on user: NO
      • Impact on build: NO
      • Impact on hardware: YES (affects arm64 architecture, potentially impacting performance or code size - needs details)
      • ...etc...
  • Testing:

    • "ci ostest" is insufficient. While CI testing is important, it should be supplemented with specific local testing details. What build host was used? What specific arm64 target board and configuration?
    • The requirement specifically asks for "Testing logs before change" and "Testing logs after change." These are missing entirely. Simply saying the change "works as intended" is not verifiable. Show evidence! Even if the change is purely cosmetic, provide some form of output demonstrating the change.

In short, the PR needs to be much more descriptive and provide concrete evidence of testing and impact assessment. Each section of the NuttX requirements needs to be addressed thoroughly.

arch/arm64/src/common/arm64_syscall.c Outdated Show resolved Hide resolved
arch/arm64/src/common/arm64_syscall.c Outdated Show resolved Hide resolved
arch/arm64/src/common/arm64_syscall.c Outdated Show resolved Hide resolved
arch/arm64/src/common/arm64_syscall.c Outdated Show resolved Hide resolved
fix the comment in apache#14980

Signed-off-by: hujun5 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit ab0fb80 into apache:master Nov 29, 2024
12 checks passed
@hujun260 hujun260 deleted the apache_38 branch December 1, 2024 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants